fix: don't abort on worker termination during module load - #1993
Open
edusperoni wants to merge 1 commit into
Open
fix: don't abort on worker termination during module load#1993edusperoni wants to merge 1 commit into
edusperoni wants to merge 1 commit into
Conversation
worker.terminate() calls Isolate::TerminateExecution() on the worker isolate from the parent thread, after which every V8 entry that runs JS hands back an empty handle. Three call sites in the worker's script-load path unwrapped those handles without checking: - ModuleInternal::LoadModule called script->Run(...).ToLocalChecked() one line before the tc.HasCaught() guard meant to handle exactly that, so a terminate landing mid-load killed the process with "Fatal error in v8::ToLocalChecked / Empty MaybeLocal". - The same function unwrapped the __extends lookup unconditionally. - CallWorkerScopeOnErrorHandle, which runs precisely when a worker script fails to load, unwrapped the global "onerror" lookup. NativeScriptException's TryCatch constructor then dereferenced tc.Message() unconditionally. A terminated TryCatch exposes no message object, so building the error to report turned the abort into a SIGSEGV, which the runtime's own signal handler converted into an opaque "JNI Exception occurred (SIGSEGV)" and no tombstone. All four now test before unwrapping, matching the sibling compile sites in LoadModule. Reporting already suppresses termination -- BackgroundLooper guards on isTerminating_ and CallWorkerScopeOnErrorHandle returns early for a terminating wrapper -- so a terminate during load unwinds as a normal shutdown. Also zero-initialises the sigaction struct used to install the SIGABRT and SIGSEGV handlers, whose sa_mask and sa_flags were stack garbage. The device suite hit this on roughly 20% of cold runs (pm clear + launch) on an arm64 emulator, always in a worker spawned by the Workers suite within the first seconds of the run; the faulting frame symbolised to ModuleInternal::LoadModule. 27 cold runs on the fixed build are clean.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Targets
feat/v8-14(#1987) so it rides the upcoming merge; the bugs themselves are long-standing and exist onmainunchanged (verified — every fixed site is byte-identical there).Intermittent process aborts/crashes (~20% of cold full-suite runs on an emulator, always in the worker-heavy early phase) were root-caused to
worker.terminate()landing while the worker is still loading its script. OnceTerminateExecution()is armed, every V8 entry that runs JS returns an emptyMaybeLocal— and three call sites on exactly this path unwrapped without checking. The proven signature (symbolized from a device tombstone) isFatal error in v8::ToLocalChecked / Empty MaybeLocalon a worker thread, aborting fromModuleInternal::LoadModule.Fixes
ModuleInternal.cpp—script->Run()was unwrapped withToLocalChecked()one line before thetc.HasCaught()guard meant to handle it; now checked first (matching the sibling compile sites in the same function). Same treatment for the unconditional__extendsglobal lookup a few lines down.NativeScriptException.cpp— theTryCatchconstructor now bails out early ontc.HasTerminated() || tc.Message().IsEmpty()with a fallback message, and no longer builds aPersistentfrom an empty exception handle (which leftReThrowToV8dereferencing an emptyLocal).CallbackHandlers.cpp—CallWorkerScopeOnErrorHandleunwrapped the globalonerrorlookup withToLocalChecked(); it runs precisely when a worker script fails to load. NowToLocal()+ early return.Runtime.cpp—struct sigactionwas never zero-initialized, leavingsa_mask/sa_flagsas stack garbage for both signal handlers (a source of run-to-run nondeterminism in crash behavior).No behavior change for healthy paths: termination reporting was already correctly suppressed (
isTerminating_is set beforeTerminateExecution()and guarded inBackgroundLooper/CallWorkerScopeOnErrorHandle); no existing spec asserts an error for terminate-during-load.Known remaining flake (out of scope): while re-verifying the reproduction on the unfixed build, one crash of a different signature was captured — a bionic
Pointer tag ... was truncatedSIGABRT on a worker thread right after isolate creation (heap corruption, not an empty-handle abort). It was not observed in 27 cold runs of the fixed build, but this PR does not claim to fix it; it's an open follow-up.Related Pull Requests
Does your pull request have unit tests?
Yes —
tests/testWorkerTerminateDuringLoad.js: the worker spins at module scope soterminate()deterministically lands inside the module-function call (the wide window; thescript->Run()window is microseconds and can't be hit reliably), asserting noonerrorfires and the process survives. It cannot fail spuriously. It lives in the app'stests/becauseshared/Workers/is a submodule shared with iOS. Suite: 606 specs, 0 failures (605 baseline + 1).Statistical verification: unfixed build reproduces at ~20% per cold full-suite run (re-armed before fixing: 1/5); fixed build ran 27 cold full-suite runs with zero crashes and a clean dropbox/tombstone sweep.